[KDESKTOP-1262] Do not force file status after an upload session abortion #366
Conversation
luc-guyot-infomaniak
force-pushed
the
kdesktop-1262-do-not-force-status-after-uploas-session-abortion
branch
from
f22ce02 to
e26d2fc
Compare
| const SyncPath &localPath = getFilePath(); | ||
| static const std::vector<const char *> extendedAttributes = {EXT_ATTR_STATUS, EXT_ATTR_PIN_STATE}; | ||
|
|
||
| for (const auto attribute: extendedAttributes) { |
There was a problem hiding this comment.
Questions.
- Should we remove all attributes or just
EXT_ATTR_STATUS? (What is the definition of a placeholder for the application?) - Is a missing permission an actual problem for the user?
- In case of unexpected failure, should the exit code be changed?
There was a problem hiding this comment.
Should we remove all attributes or just EXT_ATTR_STATUS? (What is the definition of a placeholder for the application?)
A file is a placeholder if it has the EXT_ATTR_STATUS extended attribute (see LiteSyncExtConnector::vfsGetStatus). So it should be enough to remove just this one. However, there is no point to keep only the pin state.
Moreover, the definition of a placeholder based solely on the status is also arguable. Perhaps basing it on the pinstate instead could help us display the correct status in Finder without risking to allow the deshydration of a not yet synced item.
There was a problem hiding this comment.
Is a missing permission an actual problem for the user?
Maybe. Perhaps we lose permissions during upload, which caused the abort. So the status has been set but cannot be removed, meaning that the file could be dehydrated while not synced, therefor corrupted.
There was a problem hiding this comment.
In case of unexpected failure, should the exit code be changed?
Maybe again ^^ but what can we do if the access rights have been removed 🥲
luc-guyot-infomaniak
added
3.6.8
Review required but do not merge yet
luc-guyot-infomaniak
requested review from
ChristopheLarchier,
ClementKunz and
herve-er
and removed request for
ClementKunz
…ad session abortion
luc-guyot-infomaniak
force-pushed
the
kdesktop-1262-do-not-force-status-after-uploas-session-abortion
branch
from
740245b to
87090d5
Compare
|
| static const std::vector<const char *> extendedAttributes = {EXT_ATTR_STATUS, EXT_ATTR_PIN_STATE}; | ||
|
|
||
| for (const auto attribute: extendedAttributes) { |
There was a problem hiding this comment.
This should be in a utility function outside this class. Especially because normalupload (meaning direct uploads, not upload session) are also concerned.
| @@ -16,8 +16,8 @@ | |||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | |||
| */ | |||
|
|
|||
| #import "../../../../src/libcommonserver/io/fileAttributes.h" | |||
There was a problem hiding this comment.
Move the common declarations in vfs.h ?
Or create a new dynamic library "libcommonserverext" with the common code.
| // by the application when restarting the synchronization. | ||
|
|
||
| const SyncPath &localPath = getFilePath(); | ||
| static const std::vector<const char *> extendedAttributes = {EXT_ATTR_STATUS, EXT_ATTR_PIN_STATE}; |
There was a problem hiding this comment.
Use the new function IoHelper::removeLiteSyncXAttrs
ClementKunz
removed
the
Review required but do not merge yet
This pull request aims at addressing KDESKTOP-1262:
vfsForceStatuswithnullptrafter a drive upload session abortion.